Only gossip connected peers #38
Conversation
📝 WalkthroughWalkthroughThe pull request introduces significant modifications to the Changes
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.61.0)level=warning msg="[config_reader] The configuration option Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
p2p/discovery.go (1)
215-227: Enhance error handling and logging.While the connection check is a good addition, consider these improvements:
- Add specific error type checks for better error handling
- Include more context in error logs
if len(pd.host.Network().ConnsToPeer(p.ID)) == 0 { err := pd.host.Connect(ctx, p) if err != nil { + // Check for specific error types + if errors.Is(err, context.DeadlineExceeded) { + pd.logger.Warn(). + Err(err). + Stringer("to", p.ID). + Msg("Connection attempt timed out") + return + } pd.logger.Error().Err(err). Stringer("to", p.ID). + Int("known_addrs", len(p.Addrs)). Msg("Failed to connect to peer") return } pd.logger.Debug(). Stringer("to", p). + Int("addr_count", len(p.Addrs)). Msg("Connected to peer") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
p2p/discovery.go(5 hunks)
🔇 Additional comments (3)
p2p/discovery.go (3)
Line range hint 97-108: LGTM! Improved method naming.
The renaming from GetPeers to GetKnownPeers better reflects the method's purpose and distinguishes it from the new GetConnectedPeers method.
157-159: LGTM! Improved peer sharing logic.
The change to share only connected peers aligns with the PR objective and improves the reliability of peer discovery by ensuring only connectable peers are shared.
Line range hint 1-277: Verify impact on connection handling performance.
The changes to only gossip connected peers and prevent reconnections could impact the network's ability to maintain connections under certain conditions.
✅ Verification successful
Connection handling implementation is robust and well-designed
The codebase demonstrates a solid implementation of connection handling with:
- Appropriate timeout configurations (10s for gossip operations)
- Proper error handling and logging for connection failures
- Built-in concurrency control with
MaxGossipConcurrency - Smart connection logic that prevents redundant connections
- Graceful connection management through context cancellation
The changes to only gossip connected peers and prevent reconnections are actually beneficial:
- Reduces network overhead by avoiding redundant connections
- Prevents connection thrashing when addresses change
- Maintains stable connections through proper timeout and error handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential connection handling issues in the codebase
# Look for connection timeout configurations
rg -A 2 "context.*WithTimeout|SetDeadline|SetReadDeadline|SetWriteDeadline"
# Check for connection retry logic
rg -A 5 "Connect.*retry|reconnect|backoff"
# Look for connection error handling patterns
rg -A 3 "Failed to connect|connection.*failed|connection.*error"
Length of output: 4880
| // GetConnectedPeers returns all currently connected peers | ||
| func (pd *PeerDiscovery) GetConnectedPeers() []peer.AddrInfo { | ||
| conns := pd.host.Network().Conns() | ||
| peerMap := make(map[peer.ID]peer.AddrInfo) | ||
|
|
||
| for _, conn := range conns { | ||
| remotePeer := conn.RemotePeer() | ||
| remoteAddr := conn.RemoteMultiaddr() | ||
|
|
||
| if peerInfo, exists := peerMap[remotePeer]; exists { | ||
| // peer already in map, add the new address if it's not already there | ||
| if !multiaddr.Contains(peerInfo.Addrs, remoteAddr) { | ||
| peerInfo.Addrs = append(peerInfo.Addrs, remoteAddr) | ||
| peerMap[remotePeer] = peerInfo | ||
| } | ||
| } else { | ||
| // new peer, add to map | ||
| peerMap[remotePeer] = peer.AddrInfo{ | ||
| ID: remotePeer, | ||
| Addrs: []multiaddr.Multiaddr{remoteAddr}, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // flatten map | ||
| peers := make([]peer.AddrInfo, 0, len(peerMap)) | ||
| for _, peerInfo := range peerMap { | ||
| peers = append(peers, peerInfo) | ||
| } | ||
|
|
||
| return peers | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding concurrency protection and error handling.
While the implementation is functionally correct, consider these improvements:
- Add mutex protection as the method accesses shared network state
- Add error handling for edge cases (e.g., connection state changes during iteration)
func (pd *PeerDiscovery) GetConnectedPeers() []peer.AddrInfo {
+ pd.mu.RLock()
+ defer pd.mu.RUnlock()
+
conns := pd.host.Network().Conns()
peerMap := make(map[peer.ID]peer.AddrInfo)
for _, conn := range conns {
+ // Skip if connection is closing
+ if conn.Stat().Direction == network.DirUnknown {
+ continue
+ }
remotePeer := conn.RemotePeer()
remoteAddr := conn.RemoteMultiaddr()Committable suggestion skipped: line range outside the PR's diff.
|
As we plan to drop peer discovery via gossip, shall we close this PR? |
We should only gossip peers that are actually connectable.
Also only connect to peer if there are not any connections.
Summary by CodeRabbit
New Features
Improvements